end-to-end tests in circleci [CUMULUS-371] #238
Conversation
# Conflicts: # CHANGELOG.md
scisco
changed the title
packages/common/aws.js
Outdated
| * @param {integer} receiptHandle - the unique identifier of the sQS message | ||
| * @returns {Promise} an AWS SQS response | ||
| */ | ||
| exports.deleteSQSMessage = async (queueUrl, receiptHandle) => { |
There was a problem hiding this comment.
There's no await here so I don't think the async declaration is necessary
tests/sftp_pdr_parse_ingest.js
Outdated
| const filename = 'cumulus-message-adapter.zip'; | ||
| context.src = path.join(process.cwd(), 'tests', `${randomString()}.zip`); | ||
| context.dest = path.join(process.cwd(), 'tests', randomString()); | ||
| await fetchMessageAdapter(null, gitPath, filename, context.src, context.dest); |
There was a problem hiding this comment.
Since these lines of code are duplicated in tests/ftp_pdr_parse_ingest.js could we extract it into a function intestUtils (or the fetchMessageAdapter function itself)?
| @@ -0,0 +1,55 @@ | |||
| { | |||
| "DiscoverPdrs": { | |||
There was a problem hiding this comment.
Minor style thing but elsewhere in this repo we're using 2 spaces for indentation so preference would be to be consistent with 2 spaces. Here and in other .json files in this PR
|
|
||
| let stepInput = clone(message); | ||
|
|
||
| for (const step of workflow.steps) { |
There was a problem hiding this comment.
Curious - is there a reason to use this iteration syntax over workflowSteps.forEach((step) => { ?
There was a problem hiding this comment.
This has to happen in sequence. If we don't use for of, we have to use some other library to ensure each async task happen after the other one ended.
There was a problem hiding this comment.
I see that makes sense
packages/integration-tests/local.js
Outdated
| let stepInput = clone(message); | ||
|
|
||
| for (const step of workflow.steps) { | ||
| stepInput = await runStep(step.lambda, step.handler, stepInput, step.name) |
There was a problem hiding this comment.
Nit: missing trailing semi-colon
| } | ||
| trail.output = clone(stepInput); | ||
|
|
||
| return trail; |
There was a problem hiding this comment.
I don't think trail is a promise, as documentation suggests is returned from this function but I could be missing something
There was a problem hiding this comment.
this is an async/await function so it always return a promise.
| task[moduleFunctionName](message, {}, (e, r) => { | ||
| if (e) return reject(e); | ||
| console.log(`Completed execution of ${stepName}`); | ||
| return resolve(r); |
There was a problem hiding this comment.
I don't think a return is necessary before reject and resolve since this function returns the Promise itself
There was a problem hiding this comment.
it's good practice to return something in a function if possible.
packages/integration-tests/local.js
Outdated
| message.cumulus_meta.task = stepName; | ||
|
|
||
| try { | ||
| // add message adapter to task folder |
There was a problem hiding this comment.
Is this comment out of date? Looks add message adapter is done explicitly in calling copyCMAToTask in tests
packages/integration-tests/local.js
Outdated
| const dest = path.join(taskFullPath, 'cumulus-message-adapter'); | ||
| let resp; | ||
|
|
||
| process.env.CUMULUS_MESSAGE_ADAPTER_DIR = dest; |
There was a problem hiding this comment.
Is this being used? I could be missing something but it seems like all this message adapter work went into copyCMAToTask and some of these lines could be removed.
There was a problem hiding this comment.
yeah, that is used inside the python code
| * @param {Array} cfOutputs - mocked outputs of a CloudFormation template | ||
| * @returns {Object} the generated cumulus message | ||
| */ | ||
| function messageBuilder(workflow, configOverride, cfOutputs) { |
There was a problem hiding this comment.
Non-blocking but here and elsewhere in this PR it could be more maintainable to use an object argument, e.g.
function messageBuilder({workflow, configOverride, cfOutputs})
this makes it easier to add arguments without being concerned about what order they are in when you make the function call.
|
|
||
| return messages.Messages; | ||
| } | ||
| return []; |
There was a problem hiding this comment.
I think this function just returns an array of messages (or the empty array) (e.g. not {Promise.<Array>} as documentation above indicates
There was a problem hiding this comment.
async/await function always return a promise.
There was a problem hiding this comment.
You're right! I learned something.
Summary: Summary of changes
Addresses CUMULUS-371
Changes
Test Plan
Things that should succeed before merging.